8364547: Window size may be incorrect when constrained to min or max #1870

beldenfox · 2025-08-14T16:26:07Z

When changing the width and height of a window the platform code is responsible for enforcing the min and max size constraints. If the final width and height don't match the width and height passed into setBounds the platform needs to call notifyResize to correct the window's properties. This happens naturally if the window size actually changes since that will trigger the OS to send size change notifications. If the platform window size doesn't change the OS notifications won't trigger. We need to catch that case and send notifyResize anyway.

This PR does this for Mac and Windows. Linux is being handled in PR #1789 which also includes the system tests for these bugs.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

JDK-8364547: Window size may be incorrect when constrained to min or max (Bug - P4)

Reviewers

Kevin Rushforth (@kevinrushforth - Reviewer)
Andy Goryachev (@andy-goryachev-oracle - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1870/head:pull/1870
$ git checkout pull/1870

Update a local copy of the PR:
$ git checkout pull/1870
$ git pull https://git.openjdk.org/jfx.git pull/1870/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1870

View PR using the GUI difftool:
$ git pr show -t 1870

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1870.diff

Using Webrev

Link to Webrev Comment

…roperties

bridgekeeper · 2025-08-14T16:26:54Z

👋 Welcome back mfox! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2025-08-14T16:28:29Z

@beldenfox This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8364547: Window size may be incorrect when constrained to min or max

Reviewed-by: kcr, angorya

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 1 new commit pushed to the master branch:

5d06943: 8368478: RichTextArea: add IME support

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

mlbridge · 2025-08-14T16:31:33Z

Webrevs

kevinrushforth

This looks reasonable although I haven't tested it. I left one question inline. I presume you have run all systems tests?

kevinrushforth · 2025-08-15T16:53:47Z

modules/javafx.graphics/src/main/java/com/sun/glass/ui/win/WinWindow.java

+            if (minMaxEnforced) {
+                notifyResize(WindowEvent.RESIZE, pw, ph);
+            }


Could this lead to two resize commands in some cases? I can see why this new logic is needed for the case where the window was already at max (or min) width and height, but if it wasn't already constrained, wouldn't the resize event have already happened?

The issue is that the Java side updates the property first, and only afterward requests the Glass native side to apply the change. I've found this to cause many problems and I fixed the same way in #1789 - when the change can't be applied, it notifies back.

Could this lead to two resize commands in some cases?

Yes, I was trying not to be too clever with my checks. I figured if the second notification wasn't necessary it would be benign. At the very least it won't trigger invalidation of the window's width and height properties.

I will tighten this up since I have to tweak the code a bit anyway. I just verified that on Windows you can alter the size of a maximized window and the OS will keep it in the MAXIMIZED state (as it resizes glass calls notifyResize with WindowEvent.MAXIMIZED). This PR can kick the window out of the MAXIMIZED or MINIMIZED state incorrectly. Unfortunately it puts the window in the wrong internal state without updating the maximized or iconified properties so it's not easy to write a test to detect this.

This is pending work from you, right? I'll put it back on my review queue once you make the changes you mentioned above.

I've made the changes. It should be ready for review.

I completely missed that you had already done this. I'll review it then.

kevinrushforth · 2025-08-15T16:56:08Z

/reviewers 2

openjdk · 2025-08-15T16:58:24Z

@kevinrushforth
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

bridgekeeper · 2025-09-15T22:29:01Z

@beldenfox This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a /touch or /keepalive command to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

beldenfox · 2025-09-16T20:47:59Z

/touch

openjdk · 2025-09-16T20:48:36Z

@beldenfox The pull request is being re-evaluated and the inactivity timeout has been reset.

kevinrushforth

The fix looks good.

Windows: I ran SizingTest from PR #1789 (removing the assumeTrue that skips the test on Windows), and it fails for me without this fix and passes with this fix.

macOS: I ran SizingTest from PR #1789, and I get 8 test failures without this fix and 4 failures with this fix. The 4 failures are all using StageStyle.EXTENDED, which might be a different bug.

@beldenfox Can you merge in the latest master (I did when testing and there were no issues)? And can you coment on the StageStyle.EXTENDED failures on macOS? I think it would be fine to treat them as a separate bug unless it is easy to fix.

I'll reapprove if you make changes.

kevinrushforth · 2025-10-15T21:04:50Z

@lukostyra or @andy-goryachev-oracle Can one of you be the second reviewer?

beldenfox · 2025-10-17T13:28:06Z

Thanks for the review. I'm traveling so won't be able to pick this up again until November.

andy-goryachev-oracle

tested on macOS 15.7.1 M1
the reproducer works
found no ill effects in the monkey tester.

beldenfox · 2025-10-31T16:17:20Z

There was a request to merge in the latest master but this PR is also marked as ready. Should I go ahead with the merge?

The SizingTest failures on macOS with StageStyle.EXTENDED are due to some weirdness in the OS. When we set the minimum size on an EXTENDED window the OS simply adds 16 units to the minimum height (I can verify this by writing the minSize and then immediately reading it back). I'm still trying to figure out where this magic number comes from. For EXTENDED we add a toolbar but the minimum size of a toolbar is 28 units. In any case I think this is a separate issue. I'll do a bit more research and file a bug.

andy-goryachev-oracle · 2025-10-31T16:24:08Z

there are no merge conflicts, you can integrate.

kevinrushforth · 2025-10-31T18:45:42Z

A primary reason for merging master in a PR whose source branch is out of date is to catch any problems that might arise due to semantic conflicts. Skara and GitHub do a pretty good job of catching merge conflicts, but there can be semantically conflicting changes even if there are no git merge conflicts.

This is 70 commits behind, so merging master is a good idea. It will trigger a GHA run, which isn't conclusive, but is at least a first pass indicator. If it is a clean merge, there will be no need to re-review.

Having said that, I did my testing using a branch in my repo where I merged in master as of two weeks ago (I always do that when I run CI tests) and didn't see any problems, so if you're confident that there won't be a problem, you can integrate.

beldenfox · 2025-10-31T22:59:01Z

/integrate

openjdk · 2025-10-31T23:00:14Z

Going to push as commit c77c233.
Since your change was applied there has been 1 commit pushed to the master branch:

5d06943: 8368478: RichTextArea: add IME support

Your commit was automatically rebased without conflicts.

openjdk · 2025-10-31T23:00:27Z

@beldenfox Pushed as commit c77c233.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

beldenfox added 4 commits April 30, 2025 11:34

Notify WindowStage if setBounds didn't change the size as expected

96bf8d8

Comment cleanup

5de1c38

Merge remote-tracking branch 'upstream/master' into sendcorrectsize

5907c0c

If min/max window size enforced send notification to correct Window p…

1c45278

…roperties

openjdk bot added the rfr Ready for review label Aug 14, 2025

kevinrushforth reviewed Aug 15, 2025

View reviewed changes

Remove double notifications, keep window min/max/normal state unchanged.

468fd35

kevinrushforth self-requested a review August 18, 2025 20:29

beldenfox mentioned this pull request Sep 26, 2025

8354943: [Linux] Simplify and update glass gtk backend: window sizing, positioning, and state management issues #1789

Open

3 tasks

kevinrushforth approved these changes Oct 15, 2025

View reviewed changes

andy-goryachev-oracle approved these changes Oct 28, 2025

View reviewed changes

openjdk bot added the ready Ready to be integrated label Oct 28, 2025

Merge remote-tracking branch 'upstream/master' into sendcorrectsize

c270bff

openjdk bot added the integrated Pull request has been integrated label Oct 31, 2025

openjdk bot closed this Oct 31, 2025

openjdk bot removed ready Ready to be integrated rfr Ready for review labels Oct 31, 2025

8364547: Window size may be incorrect when constrained to min or max #1870

8364547: Window size may be incorrect when constrained to min or max #1870

Uh oh!

Conversation

beldenfox commented Aug 14, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Aug 14, 2025

Uh oh!

openjdk bot commented Aug 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mlbridge bot commented Aug 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

kevinrushforth left a comment

Choose a reason for hiding this comment

Uh oh!

kevinrushforth Aug 15, 2025

Choose a reason for hiding this comment

Uh oh!

tsayao Aug 15, 2025

Choose a reason for hiding this comment

Uh oh!

beldenfox Aug 15, 2025

Choose a reason for hiding this comment

Uh oh!

kevinrushforth Oct 10, 2025

Choose a reason for hiding this comment

Uh oh!

beldenfox Oct 11, 2025

Choose a reason for hiding this comment

Uh oh!

kevinrushforth Oct 11, 2025

Choose a reason for hiding this comment

Uh oh!

kevinrushforth commented Aug 15, 2025

Uh oh!

openjdk bot commented Aug 15, 2025

Uh oh!

bridgekeeper bot commented Sep 15, 2025

Uh oh!

beldenfox commented Sep 16, 2025

Uh oh!

openjdk bot commented Sep 16, 2025

Uh oh!

kevinrushforth left a comment

Choose a reason for hiding this comment

Uh oh!

kevinrushforth commented Oct 15, 2025

Uh oh!

beldenfox commented Oct 17, 2025

Uh oh!

andy-goryachev-oracle left a comment

Choose a reason for hiding this comment

Uh oh!

beldenfox commented Oct 31, 2025

Uh oh!

andy-goryachev-oracle commented Oct 31, 2025

Uh oh!

kevinrushforth commented Oct 31, 2025

Uh oh!

beldenfox commented Oct 31, 2025

Uh oh!

openjdk bot commented Oct 31, 2025

Uh oh!

openjdk bot commented Oct 31, 2025

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

beldenfox commented Aug 14, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Aug 14, 2025 •

edited

Loading

mlbridge bot commented Aug 14, 2025 •

edited

Loading